Skip to content

Add coronary artery segmentation bundle (CoronSegmentator)#761

Open
Foxconn-bgroup-aidd wants to merge 31 commits into
Project-MONAI:devfrom
Foxconn-bgroup-aidd:760-CoronSegmentator
Open

Add coronary artery segmentation bundle (CoronSegmentator)#761
Foxconn-bgroup-aidd wants to merge 31 commits into
Project-MONAI:devfrom
Foxconn-bgroup-aidd:760-CoronSegmentator

Conversation

@Foxconn-bgroup-aidd
Copy link
Copy Markdown

@Foxconn-bgroup-aidd Foxconn-bgroup-aidd commented Jul 16, 2025

proposed Model: CoronSegmentator

This PR adds a new coronary artery segmentation model named coronsegmentator under models/coronsegmentator/.

Coronsegmentator is an automated pipeline that performs dual-task segmentation on cardiac CT images, focusing on both whole-heart and coronary artery structures. It integrates MONAI’s Auto3DSeg for general cardiac segmentation and a custom nnU-Net model for detailed coronary artery segmentation. The pipeline further converts segmentation results (in STL format) into USD files for downstream 3D visualization and digital twin simulation using NVIDIA Omniverse.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added CoronSegmentator bundle for coronary artery segmentation from CT images using MONAI ConfigWorkflow
    • Integrated automated segmentation pipeline with nnU-Net model and 3D visualization export support
  • Tests

    • Added unit tests to validate the inference configuration workflow
  • Documentation

    • Added README with usage instructions and dataset references
    • Added licensing and data attribution information

@Nic-Ma Nic-Ma requested review from KumoLiu and yiheng-wang-nv July 21, 2025 16:22
Comment thread models/CoronSegmentator/.gitignore Outdated
@@ -0,0 +1,7 @@
Input/
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Foxconn-bgroup-aidd , can we remove this file?
A part of files are already ignored (set in https://github.com/Project-MONAI/model-zoo/blob/dev/.gitignore), and for customized folder like Input/ and Output/, I think we can remove them directly. What do you think?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no problem removing them. Thanks.

@@ -0,0 +1,17 @@
{
"bundle_root": "./",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you prepare a unit test script?
It's helpful to check if (1) large files are downloadable (2) inference pipeline works (3) dependencies mentioned in metadata are correct and enough to run the script.

You can refer to this example to prepare the script: https://github.com/Project-MONAI/model-zoo/blob/dev/ci/unit_tests/test_spleen_ct_segmentation.py

Copy link
Copy Markdown

@tylin7111095022 tylin7111095022 Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yiheng-wang-nv, I prepare the script refer to above url. but when I try to run the script , I got the error: ModuleNotFoundError: Cannot locate class or function path: 'scripts.heart_digital_twin.CoroSegmentator_Pipeline'. It seems to can't access the custom class is defined in inference.json. I put the script in ci/unitest/ folder and run the script at model_zoo root directory. How can I solve the problem? Thanks for answer. Following is revised inference.json:

{
  "imports": ["$import glob"],
  "bundle_root": "./",
  "ckpt_dir": "$@bundle_root + 'models'",
  "dataset_dir": "",
  "data": "$list(sorted(glob.glob(@dataset_dir + '/*.nii.gz')))",
  "output_ext": ".usd",
  "output_dir": "$@bundle_root + 'Output'",
  "device": "$torch.device('cuda:0' if torch.cuda.is_available() else 'cpu')",
  "inferer": {
    "_target_": "scripts.heart_digital_twin.CoroSegmentator_Pipeline",
    "input_params": {
      "inputFile": "$@data[0]",
      "outputDir": "$@output_dir"
    }
  },

  "dataset": {},
  "evaluator": {},
  "network_def": {},

  "run": ["$@inferer.run()"]
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Foxconn-bgroup-aidd, please try create an init.py file in the scripts directory to make it as Python packages and also put the scripts folder just under you bundle instead of under the unittest folder. Thanks.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for comment, @KumoLiu . Following is my directory tree of project

model-zoo/
|--ci/
    |--unit_tests/
        |--test_coronaryArtery_ct_segmentation.py
|--models/
    |--myBundle/
        |--large_file.yml
        |--LICENSE
        |--configs/
            |--inference.json
            |--metadata.json
        |--docs/
        |--models/
        |--scripts/
            |--__init__.py
            |--heartDitgitalTwin.py

I run the python ci/unit_tests/test_coronaryArtery_ct_segmentation.py and got the ModuleNotFoundError: Cannot locate class or function path: 'scripts.heart_digital_twin.CoroSegmentator_Pipeline'. I still try to solve the problem which pydoc.locate(scripts.heart_digital_twin.CoroSegmentator_Pipeline) can't access custom class. Thanks for help.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KumoLiu I add the path of bundle into the sys.path and solve it, Thanks!

Copy link
Copy Markdown
Collaborator

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Foxconn-bgroup-aidd, thanks for the contributions! Leave several comments inline.

@@ -0,0 +1,88 @@
{
"schema": "https://github.com/Project-MONAI/MONAI-extra-test-data/releases/download/0.8.1/meta_schema_20220324.json",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Foxconn-bgroup-aidd, could you please utilize the latest schema available at the following link:
"https://github.com/Project-MONAI/MONAI-extra-test-data/releases/download/0.8.1/meta_schema_20240725.json"

Based on the content, it appears that you are already using the most recent version, as it includes "required_packages_version." Thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"schema": "https://github.com/Project-MONAI/MONAI-extra-test-data/releases/download/0.8.1/meta_schema_20220324.json",
"schema": "https://github.com/Project-MONAI/MONAI-extra-test-data/releases/download/0.8.1/meta_schema_20240725.json",

Comment on lines +10 to +13
"monai": " 1.4.0",
"nnunetv2": "2.6.0",
"nibabel": "5.3.2",
"numpy": "2.3.1",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please double check the monai version you are using, the one mentioned above is "1.4.0"? Thanks!

Suggested change
"monai": " 1.4.0",
"nnunetv2": "2.6.0",
"nibabel": "5.3.2",
"numpy": "2.3.1",
"nnunetv2": "2.6.0",
"nibabel": "5.3.2",

Comment on lines +77 to +78
"0": "Background",
"1": "Coronary Artery"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the "num_channels" is only one for the outputs.

Comment on lines +83 to +87
"optional_packages_version": {
"monai": "1.3.2",
"torch": "2.7.0+cu126",
"numpy": "2.3.1"
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can remove this part, the "optional_packages_version" is no longer a required one in the latest schema. thanks.

Copy link
Copy Markdown
Author

@Foxconn-bgroup-aidd Foxconn-bgroup-aidd Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of monai used in our project is “1.4.0”. “optional_packages_version" will be removed.

Comment thread models/CoronSegmentator/docs/README.md Outdated
Comment on lines +45 to +46
The ImageCAS dataset is publicly available at:
https://github.com/XiaoweiXu/ImageCAS-A-Large-Scale-Dataset-and-Benchmark-for-Coronary-Artery-Segmentation-based-on-CT.git''
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to mention the dataset link in the "Data" section, what do you think?

Comment thread models/CoronSegmentator/requirements.txt Outdated
Comment on lines +174 to +183
# try:
# Step 1: Convert input to NRRD
input_nrrd = self._convert_to_nrrd(self.input_path, input_tmp_dir)
# Step 2: Run segmentation
seg_nrrd = self._run_segmentation(input_nrrd, output_tmp_dir)
# Step 3: Process results
self._process_segmentation_results(seg_nrrd, output_tmp_dir)
# except Exception as e:
# logger.error(f"Processing failed: {str(e)}")
# raise
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove the comment code here or perhaps include the try-catch logic is also make sense.

base_center = Gf.Vec3d(
(bbox_min[0] + bbox_max[0]) / 2, bbox_min[1], (bbox_min[2] + bbox_max[2]) / 2
)
# 计算平移量,使得基底中心移动到原点
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check and remove all Chinese comments inline. Thanks.

"output_ext": ".usd",
"input_file": "Input/E_CT_0000.nii.gz",
"infer": {
"_target_": "scripts.heart_digital_twin.CoroSegmentator_Pipeline",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @yiheng-wang-nv mentioned above, please include the unit test to ensure the bundle works as expected.
I didn't see this file under the scripts folder.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mis-delete this test file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have fixed the issue of incorrectly deleting test files.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I’ve pushed the updated code. Thank you for your feedback so far! Please let me know if there’s anything else I should work on.

@yiheng-wang-nv
Copy link
Copy Markdown
Collaborator

HI @Foxconn-bgroup-aidd , based on the pre-commit CI, it seems the following json files have format issues:

Input File models/CoronSegmentator/configs/metadata.json is not a valid JSON, consider using check-json
Input File models/CoronSegmentator/configs/inference.json is not a valid JSON, consider using check-json

Foxconn-bgroup-aidd and others added 8 commits September 8, 2025 15:54
Signed-off-by: Foxconn-bgroup-aidd <bgroup-aidd@foxconn.com>
Signed-off-by: Foxconn-bgroup-aidd <bgroup-aidd@foxconn.com>
Signed-off-by: Foxconn-bgroup-aidd <bgroup-aidd@foxconn.com>
Signed-off-by: Foxconn-bgroup-aidd <bgroup-aidd@foxconn.com>
@ericspod
Copy link
Copy Markdown
Member

ericspod commented Oct 7, 2025

Hi @Foxconn-bgroup-aidd we still want to accept this model so please have a look at outstanding conversations and resolve those that have been addressed. @KumoLiu and @yiheng-wang-nv can continue their review from there. Thanks!

@yiheng-wang-nv
Copy link
Copy Markdown
Collaborator

/build

"monai": " 1.5.0",
"nnunetv2": "2.6.2",
"nibabel": "5.3.2",
"numpy": "2.3.3",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Foxconn-bgroup-aidd , can you confirm the numpy version here? I saw CI error: https://github.com/Project-MONAI/model-zoo/actions/runs/18928765961/job/54041291172?pr=761#step:7:488 It may due to wrong numpy version, or the python version used in CI does not support this version. May I know which python version do you use?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, we are using Python 3.12.9, and we have already pushed a new version. Please let us know if you need any additional information

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ericspod , can you help to check this PR? We may need to enable tests on new python versions. I'm very busy on other projects, and may not have enough time for model zoo recently.

Copy link
Copy Markdown
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just very minor comments to make. @KumoLiu had mentioned a few things previously, if these have been addressed we can marked those comments as resolved. Once the tests pass we can add this.

Comment thread models/CoronSegmentator/large_file.yml Outdated
@@ -0,0 +1,11 @@
large_files:
- path: "models/cardiacModel.pt"
# url: "https://mega.nz/file/L8oz3QhA#illHJWSCmsmV5xqbXktwiu-1jUebbpJ0HSKv0hbMFCc"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these commented links.

return md5.hexdigest()


def _download(url, destination_path):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MONAI has it's own download routines which you can use instead, and which also do hash checking.

@ericspod
Copy link
Copy Markdown
Member

ericspod commented Dec 1, 2025

@KumoLiu we seem to be having an issue with space for the tests, did we have a solution for this in this repo? Thanks!

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

Walkthrough

This PR adds a complete MONAI-compatible CoronSegmentator bundle for coronary artery segmentation. The bundle provides nnUNet-based inference, STL mesh post-processing, bundle metadata/config, documentation, and ConfigWorkflow validation tests.

Changes

CoronSegmentator Bundle Implementation

Layer / File(s) Summary
Bundle Configuration and Package Structure
models/CoronSegmentator/configs/metadata.json, models/CoronSegmentator/large_file.yml, models/CoronSegmentator/scripts/cardiac_segmentation/__init__.py, models/CoronSegmentator/scripts/coronaryArtery_segmentation/__init__.py, models/CoronSegmentator/scripts/file_process/__init__.py, models/CoronSegmentator/scripts/usd_design/__init__.py
MONAI bundle metadata schema, model versioning, required package versions, network data formats, large file manifest with MD5 hashes, and Python package initialization with Apache licensing headers.
Pipeline Orchestration and Entry Point
models/CoronSegmentator/scripts/heart_digital_twin.py
CoroSegmentatorPipeline downloads model weights from manifest, manages per-input output folders, orchestrates NNUnetPredictor execution, handles exceptions with cleanup, and serves as the MONAI bundle-compatible entry point. Helper functions parse config, create output folders, and run segmentation with logging.
Segmentation and STL Post-Processing Implementation
models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py
ComponentInfo and DSU form triangle-level connected components; STLSplitter clusters mesh triangles by proximity (bounding-box prefilter + cKDTree verification) and exports separate coronary artery STLs; NNUnetPredictor sets up isolated nnUNet workspace, runs subprocess-based prediction with CPU/GPU auto-selection, and manages temp directory cleanup.
Documentation and User Guidance
models/CoronSegmentator/docs/README.md, models/CoronSegmentator/docs/data_license.txt
README describes coronary artery segmentation pipeline, .nii.gz input format, nnU-Net inference, STL output for Omniverse, MONAI bundle run command with config overrides, ImageCAS dataset attribution, references, and Apache License 2.0. Data license manifest includes dependency/library inventory.
ConfigWorkflow Validation Tests
ci/unit_tests/test_coronaryArtery_ct_segmentation.py
Parameterized unit test creates synthetic dataset (10 random NIfTI pairs), injects bundle config overrides, constructs ConfigWorkflow in infer mode, disables inference/downloads via run="[]", and validates workflow/config structure via check_workflow(..., check_properties=True).

Sequence Diagram

sequenceDiagram
  participant ConfigWorkflow as MONAI ConfigWorkflow
  participant Pipeline as CoroSegmentatorPipeline
  participant Downloader as download_url
  participant Predictor as NNUnetPredictor
  participant Splitter as STLSplitter
  ConfigWorkflow->>Pipeline: run(inputFile, outputDir)
  Pipeline->>Downloader: fetch model weights from large_file.yml
  Pipeline->>Predictor: create predictor with input NIfTI
  Predictor->>Predictor: setup temp nnUNet workspace
  Predictor->>Predictor: run nnUNetv2_predict
  Predictor->>Pipeline: return segmentation NIfTI
  Pipeline->>Splitter: process STL output
  Splitter->>Splitter: split into coronary components
  Pipeline->>Pipeline: return True (success)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A bundle of hearts, segmented with care,
nnU-Net learns the vessels with surgical flair,
STL meshes split where the arteries mend,
MONAI orchestrates—a digital twin to transcend!
Through Omniverse chambers, the coronaries shine.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description explains the model's purpose and key features, but is missing several required template items: Fixes reference, Status indicator, and most critical checkboxes for bundle requirements (codeformat, docstrings, metadata version, naming rules, package versions, descriptions, file size handling, and path checks). Complete the PR description by: (1) adding a Fixes # reference, (2) setting Status to Ready/Work in progress/Hold, (3) filling out all required checkboxes from the template, explicitly confirming verification of metadata, package versions, file size requirements, and bundle naming conventions.
Docstring Coverage ⚠️ Warning Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a new coronary artery segmentation model bundle called CoronSegmentator, which aligns with the primary objective of the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
models/CoronSegmentator/docs/data_license.txt (1)

25-29: ⚠️ Potential issue | 🟡 Minor

Dependency versions don't match configs/metadata.json.

This table lists monai 1.4.0 and nnunetv2 2.5.1, but models/CoronSegmentator/configs/metadata.json declares monai_version: 1.5.0 and required_packages_version.nnunetv2: 2.6.2. Please reconcile so that the documented third-party versions match the runtime requirements actually installed by the bundle.

📝 Suggested alignment
-monai	1.4.0	Apache-2.0	MONAI Consortium / Project MONAI
+monai	1.5.0	Apache-2.0	MONAI Consortium / Project MONAI
 usd-core	25.5	Apache-2.0	Pixar Animation Studios
-nnunetv2 (editable)	2.5.1	Apache-2.0	Fabian Isensee / Helmholtz Munich
+nnunetv2 (editable)	2.6.2	Apache-2.0	Fabian Isensee / Helmholtz Munich
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/docs/data_license.txt` around lines 25 - 29, The
versions in models/CoronSegmentator/docs/data_license.txt (monai 1.4.0, nnunetv2
2.5.1) are inconsistent with the runtime metadata in configs/metadata.json
(monai_version and required_packages_version.nnunetv2); update the table in
data_license.txt so the listed package versions match the values in
configs/metadata.json (adjust the monai and nnunetv2 entries to the
monai_version and required_packages_version.nnunetv2 declared there), or if the
metadata is wrong, update configs/metadata.json instead—ensure both files
reflect the same canonical versions.
🧹 Nitpick comments (3)
models/CoronSegmentator/configs/inference.json (2)

5-6: bundle_root: "." is fragile; resolve relative to the config file or require an override.

When the bundle is invoked from a different CWD (CI, the unit test, or monai.bundle run from the repo root), "." makes ckpt_dir and output_dir resolve under the wrong directory, and scripts.heart_digital_twin will not be importable unless the caller has manually inserted the bundle into sys.path (which is exactly the workaround the author had to apply in the unit test, per the prior review thread).

Most bundles in this repo expect bundle_root to be passed in via CLI override (e.g., --bundle_root models/CoronSegmentator). It would also be worth adding a small imports entry that inserts @bundle_root into sys.path so scripts.heart_digital_twin resolves regardless of CWD, e.g.:

-    "imports": [
-        "$import glob"
-    ],
+    "imports": [
+        "$import glob",
+        "$import sys",
+        "$sys.path.append(`@bundle_root`)"
+    ],

Also applies to: 10-10

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/configs/inference.json` around lines 5 - 6, The
config uses bundle_root: "." which breaks resolution when run from other CWDs;
change the config so bundle_root is not a hardcoded "." by requiring a CLI
override or resolving it relative to the config file (update the bundle_root
value and any derived fields like ckpt_dir and output_dir that reference
`@bundle_root`), and add an imports entry to insert `@bundle_root` into sys.path so
scripts.heart_digital_twin can be imported regardless of CWD; update references
in the config (bundle_root, ckpt_dir, output_dir, imports) to use the resolved
`@bundle_root` token rather than a literal ".".

9-11: Several declared keys appear to be dead.

  • output_ext: ".usd" (Line 9) and device (Line 11) are never referenced by inferer, run, or any other entry — CoroSegmentatorPipeline.__init__ only consumes inputFile/outputDir.
  • dataset: {}, evaluator: {}, network_def: {} (Lines 19–21) are empty objects. If they're only there to satisfy ConfigWorkflow.check_properties, please add a brief comment in the README; otherwise drop them.

If the pipeline really should obey device and write with output_ext, plumb them through to input_params so the workflow is self-consistent.

Also applies to: 19-21

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/configs/inference.json` around lines 9 - 11, The JSON
contains dead/unused keys (output_ext, device, and empty objects
dataset/evaluator/network_def) — either remove them or wire them through: if you
want the pipeline to honor device and output_ext, add them into the same
input_params object that is passed to the inferer/run so
CoronSegmentatorPipeline.__init__ (and whoever builds input_params) can consume
them (e.g., include "device" and "output_ext" in the input_params section and
ensure inferer/run forwards input_params into
CoronSegmentatorPipeline.__init__); otherwise delete output_ext and device from
the config and either remove or document the empty dataset/evaluator/network_def
entries in the README to satisfy ConfigWorkflow.check_properties. Ensure your
changes reference the config keys by name and that inferer/run and
CoronSegmentatorPipeline.__init__ are updated consistently.
models/CoronSegmentator/configs/metadata.json (1)

57-76: outputs.pred.type: "usd" does not match MONAI bundle conventions.

The type field across the model-zoo describes the tensor kind—image, probability, tuples, object, etc.—not file format. USD is a file format produced downstream from the network; the network itself outputs a segmentation tensor. Set type: "image" (with format: "segmentation") to accurately describe the tensor, and document USD export separately in the README.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/configs/metadata.json` around lines 57 - 76, The
metadata currently mislabels the model output: change outputs.pred.type from
"usd" to "image" so the tensor kind correctly reflects a segmentation output
(leave "format": "segmentation", "num_channels", "spatial_shape", etc.
unchanged), and move any USD/export-file format details into the README or an
export step documentation rather than the metadata.json; update the entry
referenced as outputs.pred.type accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ci/unit_tests/test_coronaryArtery_ct_segmentation.py`:
- Around line 55-56: The synthetic test data is invalid: fix creation of
test_image and test_label by sampling CT-intensity-range into a dtype that can
hold Hounsfield units and producing true binary labels; specifically change the
array dtype for test_image (variable name test_image) from np.int8 to np.int16
(so values sampled from low=-1024, high=1000 are represented correctly) and
change the randint call for test_label (variable name test_label) to use high=2
so it can generate 0 and 1 labels rather than all zeros.
- Around line 65-77: The test calls check_workflow(inferrer,
check_properties=True) which triggers ConfigWorkflow ->
CoroSegmentatorPipeline.run and performs heavy external work
(monai.apps.utils.download_url and NNUnetPredictor.run spawning
nnUNetv2_predict); to fix, patch/mock those external operations in the test so
only wiring is validated: either stub CoroSegmentatorPipeline.run to a no-op or
mock monai.apps.utils.download_url and subprocess.run (or the method
NNUnetPredictor.run) to return successfully without downloading or invoking
nnUNetv2_predict; ensure your mocks target the exact symbols ConfigWorkflow
(test instantiation), CoroSegmentatorPipeline.run, download_url,
NNUnetPredictor.run, and the subprocess invocation name nnUNetv2_predict so the
test runs in CI without performing network or subprocess work.

In `@models/CoronSegmentator/configs/inference.json`:
- Line 8: The config currently builds "data" eagerly via "data":
"$list(sorted(glob.glob(`@dataset_dir` + '/*.nii.gz')))" and then uses `@data`[0] in
inferer.input_params.inputFile which raises IndexError when dataset_dir is
empty; change this by deferring or guarding the lookup: remove the eager
`@data`[0] dependency and instead resolve the input file at runtime inside
CoronSegmentatorPipeline.run() (or add a safe helper that returns a
sentinel/default or raises a clear, documented error if no .nii.gz files are
found), update inferer.input_params.inputFile to reference that runtime-resolved
value (or a safe placeholder), and add prominent documentation in the config
comment that dataset_dir must be overridden or contain NIfTI files so
ConfigWorkflow.check_workflow and monai.bundle run do not crash at construction
time.

In `@models/CoronSegmentator/configs/metadata.json`:
- Around line 38-50: The metadata.json currently declares format: "hounsfield"
but sets value_range: [0, 1], which is inconsistent; either set "value_range" to
a representative HU window (e.g., the clipping window your pipeline uses such as
[-1024, 3071] or a narrower window like [-125, 275] depending on preprocessing)
while keeping format: "hounsfield", or change format to reflect pre-normalized
data (e.g., "normalized" or "float32_normalized") and update README to document
the min-max or z-score normalization; update the "value_range" field and README
accordingly and ensure any references to network_data_format elsewhere in the
repo are consistent with the chosen option.

In `@models/CoronSegmentator/docs/README.md`:
- Around line 13-15: The README's opening paragraph, Tags, and "processed by"
steps still describe dual whole-heart + coronary segmentation, but the bundle is
coronary-only (see metadata.json task: "Coronary artery ct segmentation");
update models/CoronSegmentator/docs/README.md to remove references to MONAI
Auto3DSeg and whole-heart segmentation, revise the opening paragraph to state
coronary artery–only scope, adjust the Tags list, and change the "processed by"
steps (lines referencing MONAI/Auto3DSeg) to only list the custom nnU-Net
coronary segmentation and subsequent STL→USD conversion for 3D visualization.
- Around line 45-46: The ImageCAS dataset URL in
models/CoronSegmentator/docs/README.md contains two stray single quotes at the
end ("...-on-CT.git''") which break the link; remove the extra quote characters
so the URL ends with ".git" (update the ImageCAS dataset URL line in the README
to
"https://github.com/XiaoweiXu/ImageCAS-A-Large-Scale-Dataset-and-Benchmark-for-Coronary-Artery-Segmentation-based-on-CT.git").
- Around line 41-43: The README's example `monai.bundle run` invocation is
missing required parameters, causing the `data` list to be empty and `@data[0]`
to IndexError; update the example to pass `--meta_file`, `--logging_file`, and a
non-empty `--dataset_dir` that matches the glob used in `configs/inference.json`
(ensure `dataset_dir` points to a folder containing `.nii.gz` files), e.g.
include flags for `--meta_file configs/metadata.json --logging_file
configs/logging.conf --dataset_dir /path/to/dataset`; confirm
`configs/inference.json`'s `"dataset_dir"` is not left as an empty string or
document overriding it via the CLI.

In `@models/CoronSegmentator/large_file.yml`:
- Line 6: The YAML field hash_type is defined but never passed into the
downloader; update CoroSegmentatorPipeline.run to read hash_type from each file
entry (e.g. file_info.get("hash_type", "md5")) and pass it into the download
call as download_url(url, filepath=..., hash_val=expected_hash,
hash_type=hash_type) so the declared algorithm is honored (alternatively remove
the unused YAML field if you prefer); locate this change around the run method
in CoroSegmentatorPipeline and the call site that currently supplies only
path/url/hash_val.
- Line 4: The model bundle in models/CoronSegmentator/large_file.yml uses a
Google Drive URL under the url key which is fragile for CI; upload the weights
to a stable direct-download host (e.g., MONAI-extra-test-data GitHub release or
another CDN), replace the url value with the new direct-download link, recompute
and update the corresponding hash_val in the same YAML entry to match the
uploaded file, and apply the same replacement for the other affected bundle
referenced as "9-9" so all references use stable hosts instead of Google Drive.

In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`:
- Around line 437-441: The log messages use backslash-continued f-strings which
preserve the indentation and produce unwanted spaces; locate the except blocks
catching subprocess.CalledProcessError (the logger.error calls that reference
self.input_path and e.returncode) and replace the backslash-continued f-strings
with either a single-line f-string or implicit string concatenation (e.g., f"...
segmentation inference failed with code {e.returncode}") so the message has no
embedded indentation; apply the same fix to the other two occurrences that use
logger.error with self.input_path and e.returncode.
- Around line 358-366: preprocessed_path is set inside the installed bundle
(script_path.parent) which can pollute/read-only install dirs; change it to live
under the temporary workspace like raw_path (use self.temp_dir.name, e.g.
self.preprocessed_path = os.path.join(self.temp_dir.name,
"nnUNet/nnUNet_preprocessed")) so nnUNet writes to a transient location; also
harden the model_weight construction (the list comprehension that calls
os.listdir on script_path.parent.parent.parent/"models") by first checking that
the models directory exists and using an empty list if not (or use
os.path.exists/isdir) before listing to avoid FileNotFoundError when the models/
dir is absent.

In `@models/CoronSegmentator/scripts/heart_digital_twin.py`:
- Line 9: Replace the leftover template placeholder in the module docstring at
the top of heart_digital_twin.py with a concise, accurate description of what
the module does (e.g., "Convert NIfTI (.nii/.nii.gz) medical image files into
USD scene files and generate associated heart digital twin metadata"), ensuring
the docstring reflects the script's purpose, inputs, outputs and any important
side effects; update the module-level docstring string literal (the first
triple-quoted string in the file) accordingly.
- Around line 92-104: The run() error handling silently returns False and
performs fragile cleanup; update the except block in heart_digital_twin.run to
both surface failures to the workflow (either re-raise the caught exception
after logging or raise a new exception so ConfigWorkflow sees the failure) and
replace the manual os.remove/os.rmdir loop with shutil.rmtree(seg_folder,
ignore_errors=False) (or ignore_errors=True if you prefer best-effort cleanup)
to correctly remove nested files/directories; ensure you import shutil and keep
the gc.collect() in the finally block.

---

Outside diff comments:
In `@models/CoronSegmentator/docs/data_license.txt`:
- Around line 25-29: The versions in
models/CoronSegmentator/docs/data_license.txt (monai 1.4.0, nnunetv2 2.5.1) are
inconsistent with the runtime metadata in configs/metadata.json (monai_version
and required_packages_version.nnunetv2); update the table in data_license.txt so
the listed package versions match the values in configs/metadata.json (adjust
the monai and nnunetv2 entries to the monai_version and
required_packages_version.nnunetv2 declared there), or if the metadata is wrong,
update configs/metadata.json instead—ensure both files reflect the same
canonical versions.

---

Nitpick comments:
In `@models/CoronSegmentator/configs/inference.json`:
- Around line 5-6: The config uses bundle_root: "." which breaks resolution when
run from other CWDs; change the config so bundle_root is not a hardcoded "." by
requiring a CLI override or resolving it relative to the config file (update the
bundle_root value and any derived fields like ckpt_dir and output_dir that
reference `@bundle_root`), and add an imports entry to insert `@bundle_root` into
sys.path so scripts.heart_digital_twin can be imported regardless of CWD; update
references in the config (bundle_root, ckpt_dir, output_dir, imports) to use the
resolved `@bundle_root` token rather than a literal ".".
- Around line 9-11: The JSON contains dead/unused keys (output_ext, device, and
empty objects dataset/evaluator/network_def) — either remove them or wire them
through: if you want the pipeline to honor device and output_ext, add them into
the same input_params object that is passed to the inferer/run so
CoronSegmentatorPipeline.__init__ (and whoever builds input_params) can consume
them (e.g., include "device" and "output_ext" in the input_params section and
ensure inferer/run forwards input_params into
CoronSegmentatorPipeline.__init__); otherwise delete output_ext and device from
the config and either remove or document the empty dataset/evaluator/network_def
entries in the README to satisfy ConfigWorkflow.check_properties. Ensure your
changes reference the config keys by name and that inferer/run and
CoronSegmentatorPipeline.__init__ are updated consistently.

In `@models/CoronSegmentator/configs/metadata.json`:
- Around line 57-76: The metadata currently mislabels the model output: change
outputs.pred.type from "usd" to "image" so the tensor kind correctly reflects a
segmentation output (leave "format": "segmentation", "num_channels",
"spatial_shape", etc. unchanged), and move any USD/export-file format details
into the README or an export step documentation rather than the metadata.json;
update the entry referenced as outputs.pred.type accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea410f65-0bda-4362-98e8-c5f512b64932

📥 Commits

Reviewing files that changed from the base of the PR and between 22761f0 and f807c07.

📒 Files selected for processing (12)
  • ci/unit_tests/test_coronaryArtery_ct_segmentation.py
  • models/CoronSegmentator/.gitignore
  • models/CoronSegmentator/LICENSE
  • models/CoronSegmentator/configs/inference.json
  • models/CoronSegmentator/configs/logging.conf
  • models/CoronSegmentator/configs/metadata.json
  • models/CoronSegmentator/docs/README.md
  • models/CoronSegmentator/docs/data_license.txt
  • models/CoronSegmentator/large_file.yml
  • models/CoronSegmentator/scripts/__init__.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py
  • models/CoronSegmentator/scripts/heart_digital_twin.py

Comment thread ci/unit_tests/test_coronaryArtery_ct_segmentation.py Outdated
Comment thread ci/unit_tests/test_coronaryArtery_ct_segmentation.py
Comment thread models/CoronSegmentator/configs/inference.json
Comment on lines +38 to +50
"format": "hounsfield",
"modality": "CT",
"num_channels": 1,
"spatial_shape": [
512,
512,
256
],
"dtype": "float32",
"value_range": [
0,
1
],
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

format: "hounsfield" with value_range: [0, 1] is contradictory.

Hounsfield units span roughly [-1024, 3071]. A [0, 1] range implies the inputs are min-max normalized (no longer raw HU). Please either:

  • keep format: "hounsfield" and set value_range to a representative HU window (e.g., the clipping window the pipeline uses internally), or
  • if the bundle expects pre-normalized inputs, change format accordingly and document the normalization in the README.

This also affects how downstream tooling that consumes network_data_format will preprocess data.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/configs/metadata.json` around lines 38 - 50, The
metadata.json currently declares format: "hounsfield" but sets value_range: [0,
1], which is inconsistent; either set "value_range" to a representative HU
window (e.g., the clipping window your pipeline uses such as [-1024, 3071] or a
narrower window like [-125, 275] depending on preprocessing) while keeping
format: "hounsfield", or change format to reflect pre-normalized data (e.g.,
"normalized" or "float32_normalized") and update README to document the min-max
or z-score normalization; update the "value_range" field and README accordingly
and ensure any references to network_data_format elsewhere in the repo are
consistent with the chosen option.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The images don't appear to be scaled in your scripts so the assumption is they pixel data in the Hounsfield range as described here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericspod, thanks for the confirmation. Since the pipeline operates on raw Hounsfield data, format: "hounsfield" is correct — but value_range: [0, 1] is still inconsistent with that. The value_range field should be updated to reflect the actual HU range the model expects (e.g. [-1024, 3071] for the full HU gamut, or a narrower cardiac CT window if the pipeline clips before inference).

@Foxconn-bgroup-aidd — could you update value_range in metadata.json to match the actual HU window your preprocessing/nnU-Net model uses? For example:

"value_range": [-1024, 3071]

or whatever clipping window is applied internally in NNUnetPredictor.

Comment thread models/CoronSegmentator/docs/README.md Outdated
# url: "https://mega.nz/file/L8oz3QhA#illHJWSCmsmV5xqbXktwiu-1jUebbpJ0HSKv0hbMFCc"
url: "https://drive.google.com/file/d/11-pgULhmSZyAAfZG9n1vOusWHeH-hpwE/view?usp=drive_link"
hash_val: "0e783497e94293c86995392e5fbbc790"
hash_type: "md5"
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

hash_type is declared here but never read by the consumer.

models/CoronSegmentator/scripts/heart_digital_twin.py::CoroSegmentatorPipeline.run() only extracts path, url, and hash_val from each entry and calls download_url(url, filepath=..., hash_val=expected_hash) without forwarding hash_type. MONAI's download_url defaults to MD5, so behavior happens to be correct, but the YAML field is silently ignored. Either pass it through (hash_type=file_info.get("hash_type", "md5")) or drop the field to avoid implying it has effect.

Also applies to: 11-11

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/CoronSegmentator/large_file.yml` at line 6, The YAML field hash_type
is defined but never passed into the downloader; update
CoroSegmentatorPipeline.run to read hash_type from each file entry (e.g.
file_info.get("hash_type", "md5")) and pass it into the download call as
download_url(url, filepath=..., hash_val=expected_hash, hash_type=hash_type) so
the declared algorithm is honored (alternatively remove the unused YAML field if
you prefer); locate this change around the run method in CoroSegmentatorPipeline
and the call site that currently supplies only path/url/hash_val.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest checking the has would be a good idea as described here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "download_url\|hash_val\|hash_type\|file_info\|large_file" models/CoronSegmentator/scripts/heart_digital_twin.py -A2 -B2

Length of output: 0


🏁 Script executed:

#!/bin/bash
cat models/CoronSegmentator/scripts/heart_digital_twin.py

Length of output: 3615


@ericspod, agreed. The call site confirms the issue — hash_type is read from the YAML but silently dropped:

# current (line ~87)
download_url(url, filepath=destination_path, hash_val=expected_hash)

Here is the minimal fix for models/CoronSegmentator/scripts/heart_digital_twin.py:

for file_info in download_config["large_files"]:
    rel_path = file_info.get("path")
    url = file_info.get("url")
    expected_hash = file_info.get("hash_val")
    hash_type = file_info.get("hash_type", "md5")   # ← read hash_type

    destination_path = BUNDLE_ROOT / rel_path
    destination_dir = Path(destination_path).parent
    destination_dir.mkdir(parents=True, exist_ok=True)
    download_url(url, filepath=destination_path, hash_val=expected_hash, hash_type=hash_type)  # ← forward it

This ensures that whatever algorithm is declared in large_file.yml (e.g. sha256 in the future) is actually honoured rather than silently falling back to MONAI's MD5 default.

Note (from a prior review): large_file.yml still uses a Google Drive URL. Per repo conventions, model weights should be hosted on Hugging Face (https://huggingface.co/MONAI or a maintainer's personal HF namespace) with a resolve/main direct-download link, and the hash_val should be recomputed to match the uploaded file. Worth addressing alongside this fix.


🧠 Learnings used
Learnt from: ericspod
Repo: Project-MONAI/model-zoo PR: 761
File: models/CoronSegmentator/large_file.yml:3-3
Timestamp: 2026-05-15T13:15:13.268Z
Learning: In Project-MONAI/model-zoo `large_file.yml` files, host referenced model weights on Hugging Face (either `https://huggingface.co/MONAI` or a maintainer’s personal HF namespace) instead of Google Drive. If a `large_file.yml` contains a Google Drive `url` (e.g., `*large_file.yml` entries pointing to Google Drive), replace it with a Hugging Face direct download link using the `resolve/main` pattern: `https://huggingface.co/<namespace>/<repo>/resolve/main/<file>`. After updating the URL, recompute the corresponding `hash_val` value so it matches the contents of the uploaded Hugging Face file exactly.

Comment thread models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py Outdated
Comment thread models/CoronSegmentator/scripts/heart_digital_twin.py Outdated
Comment thread models/CoronSegmentator/scripts/heart_digital_twin.py
@ericspod
Copy link
Copy Markdown
Member

Hi @Foxconn-bgroup-aidd sorry for the long delay on this one, please have a look at the comments made by us and Coderabbit and mark resolved anything that's sorted. I think there's some valid concerns about the tests that should be addressed. Thanks!

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

♻️ Duplicate comments (1)
models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py (1)

120-121: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Remove unnecessary bare except that only re-raises.

The except Exception as e: raise e pattern adds no value. Either handle the exception or let it propagate naturally.

♻️ Proposed fix
             # Convert to STL
             temp_stl = temp_nrrd.replace(".nrrd", ".stl")
             FileConversion.convert_nrrd_to_stl(temp_nrrd, temp_stl, gaussian_sigma=1.2)

             # Move to final output
             shutil.move(temp_stl, os.path.join(self.output_path, f"{name}.stl"))

             # Cleanup temporary files
             os.remove(temp_nrrd)
-        except Exception as e:
-            raise e
         finally:
             gc.collect()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py` around
lines 120 - 121, The catch block in cardiac_seg.py that reads "except Exception
as e: raise e" is redundant and should be removed so exceptions propagate
naturally; locate the try/except surrounding the cardiac segmentation logic (the
block containing "except Exception as e: raise e") and either delete the entire
except clause or replace it with meaningful error handling or a finally block
for cleanup, ensuring you do not re-raise the exception in a way that loses the
original stack trace.
🧹 Nitpick comments (7)
models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py (1)

125-125: Remove commented-out code.

The commented line # meshes = [self.load_mesh(file) for file in self.input_files] serves no purpose and should be removed.

♻️ Proposed fix
             with concurrent.futures.ThreadPoolExecutor() as executor:
                 meshes = list(executor.map(self.load_mesh, self.input_files))
-            # meshes = [self.load_mesh(file) for file in self.input_files]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py` at line 125,
Remove the dead commented code line containing "# meshes = [self.load_mesh(file)
for file in self.input_files]" from usd_mesh_creator.py; locate the occurrence
referencing self.load_mesh and self.input_files (inside the USD mesh creation
routine) and delete the commented-out list comprehension so the file contains
only active, relevant code.
models/CoronSegmentator/scripts/usd_design/shader_leather_red.py (2)

391-392: Remove commented-out code.

The commented shader creation code should be removed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/usd_design/shader_leather_red.py` around
lines 391 - 392, Remove the two commented lines that reference shader creation
in shader_leather_red.py (the commented bsdf assignment and bsdf.CreateIdAttr
call); specifically delete the lines commenting out use of
self._create_shader(Sdf.Path(self.base_path), "preview_Principled_BSDF") and the
subsequent CreateIdAttr('UsdPreviewSurface') so there is no leftover commented
shader code (symbols: bsdf, _create_shader, Sdf.Path, self.base_path,
"preview_Principled_BSDF", CreateIdAttr, 'UsdPreviewSurface').

132-136: Remove commented-out code.

The commented dictionary definition serves no purpose and should be removed for clarity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/usd_design/shader_leather_red.py` around
lines 132 - 136, Remove the dead commented-out dictionary block that defines
image_texture (the lines starting with "# image_texture = { ... }"); delete
those commented lines so the file no longer contains unused commented code and
ensure there are no other stray commented definitions of image_texture or
related unused variables (texture, mono_source, brightness, contrast,
translation, scaling, rotation, invert, texture_space, clip) left behind.
models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py (3)

233-239: Simplify conditional list append pattern.

The inline conditional expression that appends to post_transforms_list is unusual and hard to read. Consider using a standard if statement.

♻️ Proposed fix
-    (
-        post_transforms_list.append(
-            KeepLargestConnectedComponentd(keys="pred", num_components=2)
-        )
-        if "whole-head" in model_file
-        else post_transforms_list
-    )
+    if "whole-head" in model_file:
+        post_transforms_list.append(
+            KeepLargestConnectedComponentd(keys="pred", num_components=2)
+        )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py`
around lines 233 - 239, The current inline conditional that attempts to append
KeepLargestConnectedComponentd to post_transforms_list is hard to read; replace
it with a normal if statement: check if "whole-head" in model_file and then call
post_transforms_list.append(KeepLargestConnectedComponentd(keys="pred",
num_components=2)); leave post_transforms_list unchanged otherwise. This keeps
the same behavior but improves readability around the post_transforms_list and
KeepLargestConnectedComponentd usage.

138-138: Use iterable unpacking instead of list concatenation.

Static analysis suggests using unpacking for better performance and clarity.

♻️ Proposed fix
-        normalize_modes = [main_normalize_mode] + list(extra_modalities.values())
+        normalize_modes = [main_normalize_mode, *extra_modalities.values()]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py`
at line 138, The line building normalize_modes currently concatenates lists;
replace the concatenation with iterable unpacking: construct normalize_modes by
starting with main_normalize_mode and unpacking extra_modalities.values() into
the same list (i.e., use main_normalize_mode followed by
*extra_modalities.values()) so the variable normalize_modes is built via
unpacking rather than list addition; locate this change where normalize_modes,
main_normalize_mode, and extra_modalities are referenced in
auto3dseg_segresnet_inference.py.

23-24: Remove commented-out code.

The commented import # from monai.utils import MetaKeys, convert_to_dst_type is unused and should be removed.

♻️ Proposed fix
-# from monai.utils import MetaKeys, convert_to_dst_type
 from monai.utils import convert_to_dst_type
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py`
around lines 23 - 24, Remove the unused commented import line referencing
MetaKeys; specifically delete the commented line "# from monai.utils import
MetaKeys, convert_to_dst_type" so only the active import "from monai.utils
import convert_to_dst_type" remains (look for the import statements at the top
where convert_to_dst_type and MetaKeys are mentioned).
models/CoronSegmentator/scripts/file_process/file_conversion.py (1)

97-101: ⚡ Quick win

Clarify the intent of dummy mesh creation for empty volumes.

When volume_data is all zeros, the code creates a degenerate triangle with three coincident vertices at origin. This produces an invalid mesh that may cause issues downstream (e.g., in USD conversion or rendering).

Consider either:

  1. Raising an error for empty volumes instead of silently creating dummy geometry, or
  2. Adding a comment explaining why a dummy mesh is necessary and how downstream code handles it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/file_process/file_conversion.py` around lines
97 - 101, The code creates a degenerate mesh when volume_data is all zeros
(dummy_vertices/dummy_faces used to build mesh_obj via trimesh.Trimesh), which
can break downstream USD conversion/rendering; update the empty-volume handling
in file_conversion.py by either raising a clear exception (e.g., ValueError or a
custom EmptyVolumeError) when volume_data contains only zeros, or if a
placeholder mesh must be produced, replace the degenerate triangle with a valid
minimal non-degenerate geometry and add a concise comment explaining why the
dummy mesh is required and how downstream code (e.g., the USD conversion path)
must treat it; ensure the change references the volume_data check and the
creation of mesh_obj so reviewers can find the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@models/CoronSegmentator/runtests.sh`:
- Line 86: The script uses unquoted variable expansions (e.g., cmdPrefix,
PY_EXE, TO_CLEAN) which can cause word-splitting/globbing; update all command
invocations to quote these expansions (for example use "${cmdPrefix}${PY_EXE}"
when invoking Python and quote "${TO_CLEAN}" in find invocations and test/remove
loops) while preserving behavior of the dryrun helper (ensure dryrun still
receives the intended full command string or change dryrun to accept an array of
args if you need to quote separately); apply this quoting pattern to the
locations that call the Python executable, the find/cleanup commands, and all
main execution blocks referenced in the review so globbing and spaces no longer
break runtime execution.
- Around line 122-124: The is_pip_installed function incorrectly uses command
substitution and unquoted $1, causing it to always succeed; change it to run the
Python check directly (invoke ${PY_EXE} -c "import sys, pkgutil; sys.exit(0 if
pkgutil.find_loader(sys.argv[1]) else 1)" "$1") and then return the command's
exit status (e.g., use return $? or use an if/then to return 0/1), ensuring the
package name argument is quoted when passed to ${PY_EXE}.
- Around line 126-183: The argument parser's case statement is missing handling
for the documented -v|--version flag, causing valid version requests to hit the
default invalid-key path; add a new case (e.g., -v|--version) inside the
while/case block that calls the existing print_usage or a new print_version
function (or echo a version variable) and then exits successfully, placing it
alongside the other flag cases before the help (-h|--help) case to ensure
correct handling without falling through to print_error_msg.

In
`@models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py`:
- Line 139: The loop using zip(keys, normalize_modes) can silently truncate if
the iterables differ in length; update that call to zip(keys, normalize_modes,
strict=True) in auto3dseg_segresnet_inference.py (the for key, normalize_mode
loop) so a ValueError is raised on length mismatch, preventing incorrect
normalization pairing.

In `@models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py`:
- Around line 141-157: The error handler incorrectly indexes the list "futures"
with a Future object (futures[future]) causing a TypeError; instead, when you
submit tasks (where you call self._process_single_segment for each item in
self.segments), build a mapping like future_to_segment =
{executor.submit(self._process_single_segment, seg_image, segment, output_dir):
segment for segment in self.segments} and then iterate over
concurrent.futures.as_completed(future_to_segment) so you can reference the
failed segment via future_to_segment[future] in the exception block (use the
same TimeoutError and generic Exception handling but log the segment from the
mapping and cancel the future as needed).

In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`:
- Around line 546-559: The current try/except in the
concurrent.futures.as_completed loop is using futures[future] (invalid) and
swallows exceptions, masking failures; update the concurrent execution logic
(where executor.submit(process_file, nii_file) and futures list are created and
iterated) to map each Future back to its source file (e.g., keep a dict
future_to_file = {executor.submit(...): nii_file}) or use executor.map and pair
results with input names, then in the except blocks log the correct filename
(use future_to_file[future]) and either re-raise or record failures and exit
non-zero after the loop so post-processing failures are not hidden; ensure you
still call future.cancel() on timeouts and handle TimeoutError specifically for
future_to_file[future].

In `@models/CoronSegmentator/scripts/download.py`:
- Around line 29-30: Wrap the YAML file read in a safe try/except around the
open(DOWNLOAD_CONFIG, "r") and yaml.safe_load call in download.py to handle
missing or malformed config: catch FileNotFoundError and raise or log a clear
error mentioning DOWNLOAD_CONFIG is missing, catch yaml.YAMLError to report
parse/format issues and include the underlying error message, and ensure the
function either exits gracefully or re-raises a descriptive exception so callers
know the config failed to load.
- Around line 32-45: The loop over download_config["large_files"] assumes the
key and required fields exist; add explicit validation before iterating: check
that download_config is a dict and contains a "large_files" key whose value is a
list, and for each file_info (in the for ... in download_config["large_files"]
loop) validate that file_info is a dict and contains the required keys ("path",
"url", "hash_val") and optional "hash_type" (default to "md5" if absent); for
invalid entries either raise a clear ValueError or skip with a warning, and only
call download_url(url=..., filepath=BUNDLE_ROOT / rel_path, hash_val=...,
hash_type=...) when validations pass.

In `@models/CoronSegmentator/scripts/file_process/file_conversion.py`:
- Around line 34-37: The convert_nrrd_to_nii function lacks error handling; wrap
the sitk.ReadImage and sitk.WriteImage calls in a try/except like
convert_nii_to_nrrd and convert_nrrd_to_stl: catch Exception as e, log or
include the exception details with a descriptive message (e.g., "Failed
converting NRRD to NIfTI in convert_nrrd_to_nii"), and either re-raise or return
a failure indicator consistent with the other conversion helpers so callers get
contextual error information.
- Around line 19-21: The project is missing runtime deps used by
models/CoronSegmentator/scripts/file_process/file_conversion.py: add numpy,
SimpleITK (sitk), trimesh, and scikit-image (skimage) to requirements.txt so
imports like "import SimpleITK as sitk", "import trimesh", "from skimage.measure
import marching_cubes", and any numpy usage resolve; pin compatible versions (or
ranges) consistent with monai>=1.0.1 and CI, update requirements.txt
accordingly, and run a quick install+import smoke test to confirm resolution.

In `@models/CoronSegmentator/scripts/usd_design/environment_create.py`:
- Line 112: The color space string passed to SetColorSpace is misspelled; locate
the call to inp.GetAttr().SetColorSpace("sRBG") and change the literal to "sRGB"
so the correct color space name is used (update any identical occurrences of the
typo within the same module to maintain consistency).
- Line 126: Rename the misspelled method cereate_ground to create_ground in the
class defined in environment_create.py and update its call site that currently
invokes cereate_ground (reported around line 186) to call create_ground instead;
search for symbol "cereate_ground" and replace both the method definition and
every invocation with "create_ground" to prevent the AttributeError at runtime.

In `@models/CoronSegmentator/scripts/usd_design/shader_leather_red.py`:
- Around line 374-377: The loop that creates inputs calls
shader.CreateInput(name, type_name) and then skips setting defaults when
data["customData"]["default"] is falsy; change that conditional to check for the
presence of the "default" key and exclude only None (e.g., use if "default" in
data["customData"] and data["customData"]["default"] is not None:) so
inp.Set(...) runs for valid falsy defaults like 0, False, or "" while still
skipping absent/null defaults.

In `@models/CoronSegmentator/scripts/usd_design/usd_create.py`:
- Around line 47-49: The except block in create_usd() currently logs USD
conversion errors (logger.error(f"USD creation failed: {str(e)}")) but swallows
them, allowing callers to proceed; modify the except handler in create_usd() to
re-raise the caught exception after logging (e.g., raise) so upstream callers
see the failure and the pipeline can stop or handle it appropriately while
preserving the logged message.

In `@models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py`:
- Around line 128-131: The loop over input_files and meshes uses
zip(self.input_files, meshes) which can silently truncate when lengths differ;
change it to use zip(self.input_files, meshes, strict=True) so a ValueError is
raised if sizes mismatch. Update the iteration where get_paths(...) and
create_usd_mesh(...) are called (the block referencing self.input_files, meshes,
get_paths, create_usd_mesh, and appending to mesh_paths) to use zip(...,
strict=True) and handle or let the raised exception propagate appropriately.

In `@models/CoronSegmentator/scripts/usd_design/usd_shader_applier.py`:
- Around line 449-451: Summary: current code skips setting defaults when the
default value is explicitly None; change the check to test for the presence of
the "default" key instead of comparing to None. Replace the condition `if
data["customData"]["default"] is not None` with a key-existence test like `if
"default" in data["customData"]` so that inp.Set(data["customData"]["default"])
is called even when the default is None; keep the surrounding `if "customData"
in data` guard and use the existing inp.Set(...) call to apply the value.

---

Duplicate comments:
In `@models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py`:
- Around line 120-121: The catch block in cardiac_seg.py that reads "except
Exception as e: raise e" is redundant and should be removed so exceptions
propagate naturally; locate the try/except surrounding the cardiac segmentation
logic (the block containing "except Exception as e: raise e") and either delete
the entire except clause or replace it with meaningful error handling or a
finally block for cleanup, ensuring you do not re-raise the exception in a way
that loses the original stack trace.

---

Nitpick comments:
In
`@models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py`:
- Around line 233-239: The current inline conditional that attempts to append
KeepLargestConnectedComponentd to post_transforms_list is hard to read; replace
it with a normal if statement: check if "whole-head" in model_file and then call
post_transforms_list.append(KeepLargestConnectedComponentd(keys="pred",
num_components=2)); leave post_transforms_list unchanged otherwise. This keeps
the same behavior but improves readability around the post_transforms_list and
KeepLargestConnectedComponentd usage.
- Line 138: The line building normalize_modes currently concatenates lists;
replace the concatenation with iterable unpacking: construct normalize_modes by
starting with main_normalize_mode and unpacking extra_modalities.values() into
the same list (i.e., use main_normalize_mode followed by
*extra_modalities.values()) so the variable normalize_modes is built via
unpacking rather than list addition; locate this change where normalize_modes,
main_normalize_mode, and extra_modalities are referenced in
auto3dseg_segresnet_inference.py.
- Around line 23-24: Remove the unused commented import line referencing
MetaKeys; specifically delete the commented line "# from monai.utils import
MetaKeys, convert_to_dst_type" so only the active import "from monai.utils
import convert_to_dst_type" remains (look for the import statements at the top
where convert_to_dst_type and MetaKeys are mentioned).

In `@models/CoronSegmentator/scripts/file_process/file_conversion.py`:
- Around line 97-101: The code creates a degenerate mesh when volume_data is all
zeros (dummy_vertices/dummy_faces used to build mesh_obj via trimesh.Trimesh),
which can break downstream USD conversion/rendering; update the empty-volume
handling in file_conversion.py by either raising a clear exception (e.g.,
ValueError or a custom EmptyVolumeError) when volume_data contains only zeros,
or if a placeholder mesh must be produced, replace the degenerate triangle with
a valid minimal non-degenerate geometry and add a concise comment explaining why
the dummy mesh is required and how downstream code (e.g., the USD conversion
path) must treat it; ensure the change references the volume_data check and the
creation of mesh_obj so reviewers can find the fix.

In `@models/CoronSegmentator/scripts/usd_design/shader_leather_red.py`:
- Around line 391-392: Remove the two commented lines that reference shader
creation in shader_leather_red.py (the commented bsdf assignment and
bsdf.CreateIdAttr call); specifically delete the lines commenting out use of
self._create_shader(Sdf.Path(self.base_path), "preview_Principled_BSDF") and the
subsequent CreateIdAttr('UsdPreviewSurface') so there is no leftover commented
shader code (symbols: bsdf, _create_shader, Sdf.Path, self.base_path,
"preview_Principled_BSDF", CreateIdAttr, 'UsdPreviewSurface').
- Around line 132-136: Remove the dead commented-out dictionary block that
defines image_texture (the lines starting with "# image_texture = { ... }");
delete those commented lines so the file no longer contains unused commented
code and ensure there are no other stray commented definitions of image_texture
or related unused variables (texture, mono_source, brightness, contrast,
translation, scaling, rotation, invert, texture_space, clip) left behind.

In `@models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py`:
- Line 125: Remove the dead commented code line containing "# meshes =
[self.load_mesh(file) for file in self.input_files]" from usd_mesh_creator.py;
locate the occurrence referencing self.load_mesh and self.input_files (inside
the USD mesh creation routine) and delete the commented-out list comprehension
so the file contains only active, relevant code.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3f11d68-9a5b-4d2e-9788-156fb41f87eb

📥 Commits

Reviewing files that changed from the base of the PR and between f807c07 and 14c0e6c.

📒 Files selected for processing (20)
  • ci/unit_tests/test_coronaryArtery_ct_segmentation.py
  • models/CoronSegmentator/configs/metadata.json
  • models/CoronSegmentator/docs/README.md
  • models/CoronSegmentator/large_file.yml
  • models/CoronSegmentator/runtests.sh
  • models/CoronSegmentator/scripts/cardiac_segmentation/__init__.py
  • models/CoronSegmentator/scripts/cardiac_segmentation/auto3dseg_segresnet_inference.py
  • models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/__init__.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py
  • models/CoronSegmentator/scripts/download.py
  • models/CoronSegmentator/scripts/file_process/__init__.py
  • models/CoronSegmentator/scripts/file_process/file_conversion.py
  • models/CoronSegmentator/scripts/heart_digital_twin.py
  • models/CoronSegmentator/scripts/usd_design/__init__.py
  • models/CoronSegmentator/scripts/usd_design/environment_create.py
  • models/CoronSegmentator/scripts/usd_design/shader_leather_red.py
  • models/CoronSegmentator/scripts/usd_design/usd_create.py
  • models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py
  • models/CoronSegmentator/scripts/usd_design/usd_shader_applier.py
✅ Files skipped from review due to trivial changes (6)
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/init.py
  • models/CoronSegmentator/large_file.yml
  • models/CoronSegmentator/configs/metadata.json
  • models/CoronSegmentator/scripts/cardiac_segmentation/init.py
  • models/CoronSegmentator/scripts/usd_design/init.py
  • models/CoronSegmentator/scripts/file_process/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • ci/unit_tests/test_coronaryArtery_ct_segmentation.py

Comment thread models/CoronSegmentator/runtests.sh Outdated
Comment thread models/CoronSegmentator/runtests.sh Outdated
Comment thread models/CoronSegmentator/runtests.sh Outdated
Comment thread models/CoronSegmentator/scripts/cardiac_segmentation/cardiac_seg.py Outdated
Comment thread models/CoronSegmentator/scripts/usd_design/environment_create.py Outdated
Comment thread models/CoronSegmentator/scripts/usd_design/shader_leather_red.py Outdated
Comment thread models/CoronSegmentator/scripts/usd_design/usd_create.py Outdated
Comment thread models/CoronSegmentator/scripts/usd_design/usd_mesh_creator.py Outdated
Comment thread models/CoronSegmentator/scripts/usd_design/usd_shader_applier.py Outdated
- Replace custom download.py with monai.apps.utils.download_url
- Add __init__.py to all script sub-packages for proper Python packaging
- Fix code style: black, isort, flake8 (max-line 120)
- Remove commented-out dead code (logger.error)
- Fix metadata.json: remove leading space in monai version
- Fix docs/README.md: remove trailing quotes
- Fix unit test: cross-platform path, int16 dtype, run=[] CI override
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (5)
models/CoronSegmentator/scripts/heart_digital_twin.py (2)

9-9: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Replace leftover docstring placeholder.

Line 9 still has template text (<brief description ...>) instead of an actual module description.

✏️ Proposed fix
-This script performs <brief description of converting NII Files to USD Files>.
+This script runs coronary artery segmentation on NIfTI CT files and prepares outputs for downstream 3D digital twin workflows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/heart_digital_twin.py` at line 9, The
module-level docstring in heart_digital_twin.py still contains the placeholder
"<brief description of converting NII Files to USD Files>"; replace that
placeholder with a concise, accurate description of what the module does (e.g.,
"Convert medical NIfTI (.nii/.nii.gz) files to USD format for heart digital twin
visualization and processing, including preprocessing, segmentation, and export
routines"), ensuring the top-of-file docstring accurately reflects the module's
purpose and any high-level behavior implemented by functions/classes in this
file.

99-108: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not swallow pipeline failures; use robust recursive cleanup.

At Line 99, except Exception logs and returns False, which can mask failures from the workflow caller. At Line 102-107, manual cleanup fails when subdirectories exist.

♻️ Proposed fix
 import argparse
 import gc
 import logging
 import os
+import shutil
 from pathlib import Path
@@
-        except Exception as e:
-            logger.error(f"Processing failed for {self.nii_file}: {str(e)}")
-            # Remove the output folder if failed
-            if os.path.exists(seg_folder):
-                for f in os.listdir(seg_folder):
-                    file_path = os.path.join(seg_folder, f)
-                    if os.path.isfile(file_path):
-                        os.remove(file_path)
-                os.rmdir(seg_folder)
-            return False
+        except Exception:
+            logger.exception(f"Processing failed for {self.nii_file}")
+            shutil.rmtree(seg_folder, ignore_errors=True)
+            raise
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/scripts/heart_digital_twin.py` around lines 99 - 108,
Current except block in the processing flow swallows errors by logging and
returning False and performs brittle file cleanup; change it to perform robust
recursive cleanup using shutil.rmtree(seg_folder) (if seg_folder exists) and
then re-raise the caught exception so the workflow/caller sees the failure;
update the except handling around the code referencing self.nii_file,
seg_folder, and logger to import/use shutil and call shutil.rmtree(seg_folder,
ignore_errors=True) (or check exists) before raising the original exception
(raise) instead of returning False.
models/CoronSegmentator/docs/README.md (2)

41-43: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Document full monai.bundle run invocation for this bundle.

The example command is too minimal for this bundle setup and can lead to empty input resolution at runtime. Please include meta_file, logging_file, bundle_root, and a non-empty dataset_dir.

📝 Suggested command update
-python -m monai.bundle run --config_file "configs/inference.json"
+python -m monai.bundle run \
+  --config_file configs/inference.json \
+  --meta_file configs/metadata.json \
+  --logging_file configs/logging.conf \
+  --bundle_root <path/to/CoronSegmentator> \
+  --dataset_dir <path/to/nifti_inputs>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/docs/README.md` around lines 41 - 43, Update the
example monai.bundle run invocation in the README so it includes the required
runtime parameters to avoid empty input resolution: replace the minimal command
shown with one that passes --meta_file (pointing to the bundle's meta.json),
--logging_file (path to the logging config), --bundle_root (the bundle
directory), and --dataset_dir (a non-empty dataset directory) so the bundle can
locate its config, logging, and input data at runtime.

13-29: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align README scope with implemented pipeline behavior.

This section still advertises whole-heart + coronary dual-task processing, but the added pipeline code in this PR executes coronary artery segmentation workflow only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@models/CoronSegmentator/docs/README.md` around lines 13 - 29, Update the
README to reflect that the implemented pipeline performs coronary artery
segmentation only (not dual-task whole-heart + coronary); remove or reword
references to MONAI Auto3DSeg and whole-heart segmentation in the summary and
steps, explicitly state the pipeline runs a pretrained nnU-Net coronary artery
segmentation followed by STL->USD conversion, and adjust the "Data" and
processing steps sections to align with the actual behavior of the
Coronsegmentator pipeline and the nnU-Net model used.
models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py (1)

367-377: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep nnUNet writable paths in temp workspace and guard missing models/ directory.

At Line 367, nnUNet_preprocessed is still placed under the installed bundle directory, which can break on read-only environments. At Line 375, os.listdir(...) can fail if models/ is absent.

♻️ Proposed fix
             self.raw_path = os.path.join(self.temp_dir.name, "nnUNet/nnUNet_raw")
-            self.preprocessed_path = os.path.join(
-                script_path.parent, "nnUNet/nnUNet_preprocessed"
-            )
+            self.preprocessed_path = os.path.join(
+                self.temp_dir.name, "nnUNet/nnUNet_preprocessed"
+            )
             self.results_path = os.path.join(
                 script_path.parent.parent.parent, "models/nnUNet_results"
             )
-            self.model_weight = [
-                os.path.join(script_path.parent.parent.parent, "models", f)
-                for f in os.listdir(
-                    os.path.join(script_path.parent.parent.parent, "models")
-                )
-                if f.split(".")[-1] == "zip"
-            ]  # check if any model weight exist
+            models_dir = os.path.join(script_path.parent.parent.parent, "models")
+            self.model_weight = (
+                [
+                    os.path.join(models_dir, f)
+                    for f in os.listdir(models_dir)
+                    if f.endswith(".zip")
+                ]
+                if os.path.isdir(models_dir)
+                else []
+            )  # check if any model weight exist
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`
around lines 367 - 377, The current assignment of self.preprocessed_path and
self.results_path places nnUNet data under the installed bundle and uses
os.listdir on a potentially missing models/ directory; change
self.preprocessed_path and self.results_path to point to a writable temporary
workspace (e.g., use tempfile.gettempdir() or a configurable workspace dir)
instead of script_path.parent or its ancestors, and update the self.model_weight
construction in the initializer to first check os.path.isdir(models_dir) before
calling os.listdir — if the models directory is missing, set self.model_weight
to an empty list (or a safe default) and avoid raising; reference the fields
self.preprocessed_path, self.results_path, and self.model_weight to locate and
update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@models/CoronSegmentator/docs/README.md`:
- Around line 41-43: Update the example monai.bundle run invocation in the
README so it includes the required runtime parameters to avoid empty input
resolution: replace the minimal command shown with one that passes --meta_file
(pointing to the bundle's meta.json), --logging_file (path to the logging
config), --bundle_root (the bundle directory), and --dataset_dir (a non-empty
dataset directory) so the bundle can locate its config, logging, and input data
at runtime.
- Around line 13-29: Update the README to reflect that the implemented pipeline
performs coronary artery segmentation only (not dual-task whole-heart +
coronary); remove or reword references to MONAI Auto3DSeg and whole-heart
segmentation in the summary and steps, explicitly state the pipeline runs a
pretrained nnU-Net coronary artery segmentation followed by STL->USD conversion,
and adjust the "Data" and processing steps sections to align with the actual
behavior of the Coronsegmentator pipeline and the nnU-Net model used.

In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`:
- Around line 367-377: The current assignment of self.preprocessed_path and
self.results_path places nnUNet data under the installed bundle and uses
os.listdir on a potentially missing models/ directory; change
self.preprocessed_path and self.results_path to point to a writable temporary
workspace (e.g., use tempfile.gettempdir() or a configurable workspace dir)
instead of script_path.parent or its ancestors, and update the self.model_weight
construction in the initializer to first check os.path.isdir(models_dir) before
calling os.listdir — if the models directory is missing, set self.model_weight
to an empty list (or a safe default) and avoid raising; reference the fields
self.preprocessed_path, self.results_path, and self.model_weight to locate and
update the code.

In `@models/CoronSegmentator/scripts/heart_digital_twin.py`:
- Line 9: The module-level docstring in heart_digital_twin.py still contains the
placeholder "<brief description of converting NII Files to USD Files>"; replace
that placeholder with a concise, accurate description of what the module does
(e.g., "Convert medical NIfTI (.nii/.nii.gz) files to USD format for heart
digital twin visualization and processing, including preprocessing,
segmentation, and export routines"), ensuring the top-of-file docstring
accurately reflects the module's purpose and any high-level behavior implemented
by functions/classes in this file.
- Around line 99-108: Current except block in the processing flow swallows
errors by logging and returning False and performs brittle file cleanup; change
it to perform robust recursive cleanup using shutil.rmtree(seg_folder) (if
seg_folder exists) and then re-raise the caught exception so the workflow/caller
sees the failure; update the except handling around the code referencing
self.nii_file, seg_folder, and logger to import/use shutil and call
shutil.rmtree(seg_folder, ignore_errors=True) (or check exists) before raising
the original exception (raise) instead of returning False.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e469b864-1211-4852-b056-9e1b24a54053

📥 Commits

Reviewing files that changed from the base of the PR and between 14c0e6c and 8ea6cf6.

📒 Files selected for processing (11)
  • ci/unit_tests/test_coronaryArtery_ct_segmentation.py
  • models/CoronSegmentator/configs/metadata.json
  • models/CoronSegmentator/docs/README.md
  • models/CoronSegmentator/large_file.yml
  • models/CoronSegmentator/runtests.sh
  • models/CoronSegmentator/scripts/cardiac_segmentation/__init__.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/__init__.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py
  • models/CoronSegmentator/scripts/file_process/__init__.py
  • models/CoronSegmentator/scripts/heart_digital_twin.py
  • models/CoronSegmentator/scripts/usd_design/__init__.py
✅ Files skipped from review due to trivial changes (5)
  • models/CoronSegmentator/scripts/file_process/init.py
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/init.py
  • models/CoronSegmentator/scripts/usd_design/init.py
  • models/CoronSegmentator/scripts/cardiac_segmentation/init.py
  • models/CoronSegmentator/configs/metadata.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • models/CoronSegmentator/large_file.yml
  • ci/unit_tests/test_coronaryArtery_ct_segmentation.py
  • models/CoronSegmentator/runtests.sh

@Foxconn-bgroup-aidd
Copy link
Copy Markdown
Author

Thanks for the detailed feedback!
All comments have been addressed in this update, and the CodeRabbitAI review has passed.

Please let us know if there’s anything else to fix.

Comment thread models/CoronSegmentator/runtests.sh Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same file as the one in the root of the repository? If so this shouldn't be here, if it's different it should go into the docs directory.

Comment thread models/CoronSegmentator/large_file.yml Outdated
@@ -0,0 +1,9 @@
large_files:
- path: "models/cardiacModel.pt"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- path: "models/cardiacModel.pt"
- path: "models/model.pt"

This is the expected model file name the testing system looks for.

@ericspod
Copy link
Copy Markdown
Member

Hi @Foxconn-bgroup-aidd please have a look at the comments above from Coderabbit regarding some minor changes, and the errors from testing which should be very quick fixes. These looks like easy fixes so we can get this merged soon hopefully.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`:
- Around line 464-467: Before calling shutil.move, check that
self.coronary_nii_file exists and is a file (os.path.exists or os.path.isfile);
if it does not exist, raise a clear, specific exception (e.g., RuntimeError or a
custom InferenceError) that indicates the nnUNetv2_predict step produced no
output rather than letting FileNotFoundError propagate or be misinterpreted by
run(); update the block around shutil.move (and references to
self.coronary_nii_file and self.output_path) to perform this check and raise the
dedicated error with a descriptive message including the expected path and
mention of nnUNetv2_predict.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b2c246ef-d79d-4d94-9210-9664bab40912

📥 Commits

Reviewing files that changed from the base of the PR and between 8ea6cf6 and 7169ed2.

📒 Files selected for processing (4)
  • models/CoronSegmentator/docs/README.md
  • models/CoronSegmentator/large_file.yml
  • models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py
  • models/CoronSegmentator/scripts/heart_digital_twin.py
✅ Files skipped from review due to trivial changes (1)
  • models/CoronSegmentator/docs/README.md

Comment on lines +464 to +467
shutil.move(
self.coronary_nii_file,
os.path.join(self.output_path, f"{self.base_name}.nii.gz"),
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Check the predicted NIfTI exists before moving it.

If nnUNetv2_predict exits 0 without writing self.coronary_nii_file, this shutil.move() raises FileNotFoundError, and run() later reports it as an input-access problem instead of an inference/output failure. Raise a dedicated error here so missing predictions are diagnosed correctly.

Proposed fix
-            shutil.move(
-                self.coronary_nii_file,
-                os.path.join(self.output_path, f"{self.base_name}.nii.gz"),
-            )
+            if not os.path.exists(self.coronary_nii_file):
+                raise RuntimeError(
+                    f"nnUNet completed without producing {self.coronary_nii_file}"
+                )
+            shutil.move(
+                self.coronary_nii_file,
+                os.path.join(self.output_path, f"{self.base_name}.nii.gz"),
+            )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@models/CoronSegmentator/scripts/coronaryArtery_segmentation/coronaryArtery_seg.py`
around lines 464 - 467, Before calling shutil.move, check that
self.coronary_nii_file exists and is a file (os.path.exists or os.path.isfile);
if it does not exist, raise a clear, specific exception (e.g., RuntimeError or a
custom InferenceError) that indicates the nnUNetv2_predict step produced no
output rather than letting FileNotFoundError propagate or be misinterpreted by
run(); update the block around shutil.move (and references to
self.coronary_nii_file and self.output_path) to perform this check and raise the
dedicated error with a descriptive message including the expected path and
mention of nnUNetv2_predict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants